fix(engine): fail on a null cwd, and route agent workdir filesystem checks through caps - #74

Follow-up to #73, which merged before its review threads were worked. Four
threads, three addressed and one declined with evidence.

1. A cwd expression resolving to null was silently ignored (P1 — fixed)

Thread PRRT_kwDOTLXaD86bfFah, src/nodes/integration/agent_request.rs:200.

Confirmed against main. declared_working_dir read the key as
cfg.get(key).filter(|v| !v.is_null()), and cfg arrives already
expression-resolved — so "cwd": "=nodes.prepare.item.json.worktree" whose
upstream path is missing became JSON null and read as absent. The loop then
fell through to working_dir, then to the agent definition's own directory,
then to whatever the harness defaults to. The step runs in a different checkout
and says nothing, which is exactly the failure the feature exists to remove.

A present-but-null value now fails the node, with a message that says why.
Regression tests: a_cwd_expression_that_resolves_to_null_fails_the_step and
a_null_cwd_does_not_fall_back_to_working_dir (the second pins the
cwd → working_dir fall-through specifically).

sub_workflow's config.workspace was checked for the same bug and does not
have it: its null filter runs on the raw config before expr::resolve, so an
expression resolving to null reaches .as_str() and already errors. Left alone.

2. Test filename policy (P1 — declined, --no-resolve)

Thread PRRT_kwDOTLXaD86bfFaj, tests/agent_workdir_e2e.rs:1.

AGENTS.md L6-L7 reads: "Keep Rust tests in files whose names end in
_tests.rs; do not add large inline test modules to production source files."
The second clause is what the rule is for, and the repo enforces it exactly
there: 127 of 127 test files under src/ end in _tests.rs, with no
exceptions.

tests/ follows a different, equally consistent convention — the integration
target is named for the scenario it drives:

• 30 of 43 files end in _e2e.rs (data_flow_e2e.rs, error_recovery_e2e.rs,
  approval_async_e2e.rs, reliability_e2e.rs, hitl_e2e.rs, …)
• 6 end in _tests.rs
• the rest are fuzz_*.rs / smoke_all_nodes.rs / reference_workflows.rs

Renaming one file to agent_workdir_e2e_tests.rs would make it the only
integration target in the repo with that suffix and would not move the
directory any closer to consistent. If the maintainer wants tests/ covered by
the rule, that is a sweep of 37 files plus an AGENTS.md clarification, not a
change to this one file — happy to do it as its own PR. Left for a human to
call.

3. src/validate.rs over the 500-line limit (P1 — fixed)

Thread PRRT_kwDOTLXaD86bfFam. Confirmed: 598 lines against the AGENTS.md L3-L5
limit of 500.

Split at a real seam rather than at line 500. validate_all is two kinds of
check interleaved: graph shape (duplicate ids, trigger count, edge integrity,
void topology, condition routing, declared inputs) and per-kind node
config (sub_workflow child reference, per-item fan-out selectors, memory
scope, dedup key, approval enums). The config half never looks at an edge —
every check in it is one node in isolation — so it moved wholesale to
src/validate/node_config.rs beside the existing agents/loops/scatter/
workdir submodules.

src/validate.rs 598 → 277; src/validate/node_config.rs 344. Public surface
unchanged (validate, validate_all, unresolved_agent_refs), no test
changes needed.

4. Route the filesystem checks through a caps capability (Major — implemented)

Thread PRRT_kwDOTLXaD86bfIBo, src/workdir.rs:127.

The point holds. The engine's own shell node is the counter-example that
proves it: shell hands config.cwd to the ShellRunner untouched, and
containment happens inside the host's ScriptPolicy under caps/host/. The
agent path did the same job with std::fs::canonicalize in the engine, so an
AgentRunner owning a remote or containerized workspace had every cwd
rejected before its harness saw the request — and the workspace itself failed
first, at workspace.canonicalize(). The existing mitigation (a run with no
workspace resolves nothing) only covers a host that never pins one; it does
nothing for a host with a valid remote workspace, which is precisely the case
raised.

Implemented, split by what is actually an outside-world effect:

• Shape — absolute vs relative, .. traversal — is string arithmetic with
  no filesystem in it. Factored out as workdir::check_shape and run first, on
  every host, always. Keeping it in the engine means a host implementation
  cannot accidentally drop the containment check that matters most.
• Existence, canonical form, directory-ness now route through
  AgentRunner::resolve_workdir(workspace, declared) -> WorkdirCheck, with
  three answers: Resolved(path) (the host's canonical path, which it asserts
  is contained), Refused(reason) (fails the step with that reason, prefixed
  with the node), and Unmanaged.

Unmanaged is the default method body, so no existing AgentRunner changes
and no existing behaviour changes: the engine checks its own disk exactly as
before. That also avoids adding a field to Capabilities, which would have
broken all ~19 struct-literal construction sites and every downstream host. It
fits the trait's existing design — run, resolve_agent, resolve_context and
resolve_tools are all defaulted the same way, each degrading to the
pre-existing behaviour.

resolve_node_dir became async (a remote check is I/O); the three call sites
were already in async fns. sub_workflow's config.workspace goes through the
same seam.

Five new unit tests in src/workdir_tests.rs cover a harness-owned remote
workspace resolving, a harness refusal, Unmanaged falling back to the local
filesystem, and the shape check running before the harness is consulted (a
host cannot bless a .. escape).

Also

• wiki/Node-Catalog.md "Where a step runs" documents the null rule and the
  filesystem split.
• CHANGELOG.md — the working-directory entry is still under [Unreleased], so
  it was amended in place rather than given a Fixed section, and
  AgentRunner::resolve_workdir / caps::WorkdirCheck added.
• Every touched file is under the 500-line limit (largest: node_config.rs at
  344, agent_request.rs at 475).

Commands run

cargo fmt --all -- --check
cargo clippy --all-targets --all-features -- -D warnings
cargo test
cargo test --all-features

All clean. Test counts, baseline → this branch:

+4 unit tests (workdir_tests), +2 e2e (agent_workdir_e2e, behind mock).
No failures, no regressions.